-
Notifications
You must be signed in to change notification settings - Fork 345
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add inline images in mandrill adapter #608
Add inline images in mandrill adapter #608
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for opening this pull request!
I took a quick glance and left a comment on the changes. So that I can better review the pull request, do you have any links to Mandrill documentation on inline attachments?
|
||
mandrill_message | ||
|> Map.put(:attachments, format_attachments(files)) | ||
|> Map.put(:images, format_attachments(images)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you know why we're calling this images
? Will inline attachments always be images or is that how mandrill categorizes inline attachments?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, of course! https://mailchimp.com/developer/transactional/api/messages/send-new-message/
Inside message attribute at the end.
Is similar to attachments with the difference of send the cid instead of the filename.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for sending that link. That's very helpful! This is the section I'm looking at:
Based on what I'm seeing, the changes in this PR seem good.
My only lingering question is: do you know if it's possible to add non-image files as inline attachments?
- If that's the case, then someone could add a non-image attachment with
content-id
but we would incorrectly put it in the:images
field. - If Mandrill only allows for images as inline attachments, then I think this PR is good to go.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added a new commit to check the content_type too before add it as inline image.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks so much for all this work! Looks good to me.
Thanks a lot for you @germsvel!! 🍻 |
This PR adds support to send to mandrill inline images.